[LoRA] Adds support for bias in LoRA #5733
Conversation
|
@Yard1 Thanks for reviewing the PR. I have added the enable_lora_bias flag (default set to false), which prevents the allocation of lora bias tensors when false. |
|
Related: #5930 |
|
@Yard1 Thanks for reviewing. I've added an e2e test for the lora_bias support. |
|
@followumesh you need to run |
|
@followumesh apologies, this needs another conflict resolution! |
| ): | ||
| self.reset_lora(index) | ||
|
|
||
| if self.tp_size > 1: | ||
| lora_a = self.slice_lora_a(lora_a) | ||
| lora_b = self.slice_lora_b(lora_b) | ||
| if bias is not None: | ||
| bias = self.slice_bias(bias) |
There was a problem hiding this comment.
The typing looks wrong here ... bias is a tensor but the method expects a list
There was a problem hiding this comment.
Here the slice_bias mimics slice_lora_b.
There was a problem hiding this comment.
Hmm OK fair enough, I guess the typing errors are preexisting.
| self, bias: List[Union[torch.Tensor, | ||
| None]]) -> List[Union[torch.Tensor, None]]: |
There was a problem hiding this comment.
Might be better to use tuple here instead of list. Clearer when there are a fixed number of elements and performance is generally better.
There was a problem hiding this comment.
Same as above, I have let it mimic slice_lora_b.
|
@njhill I have addressed your comments above. Can you please review this again? Thanks |
There was a problem hiding this comment.
Thanks @followumesh and sorry for the delay.
There's one remaining small but major thing to fix (and tests are failing due to this).
vllm/lora/models.py
Outdated
| if not self.lora_config.bias_enabled: | ||
| module_lora.bias = None | ||
| raise ValueError( | ||
| f"Adapter bias cannot be used for {module_name}" | ||
| " without --enable-lora-bias.") |
There was a problem hiding this comment.
This doesn't look right and is causing blanket lora failures. I think it should be:
| if not self.lora_config.bias_enabled: | |
| module_lora.bias = None | |
| raise ValueError( | |
| f"Adapter bias cannot be used for {module_name}" | |
| " without --enable-lora-bias.") | |
| if module_lora.bias is not None and not self.lora_config.bias_enabled: | |
| raise ValueError( | |
| f"Adapter bias cannot be used for {module_name}" | |
| " without --enable-lora-bias.") |
| ): | ||
| self.reset_lora(index) | ||
|
|
||
| if self.tp_size > 1: | ||
| lora_a = self.slice_lora_a(lora_a) | ||
| lora_b = self.slice_lora_b(lora_b) | ||
| if bias is not None: | ||
| bias = self.slice_bias(bias) |
There was a problem hiding this comment.
Hmm OK fair enough, I guess the typing errors are preexisting.
Signed-off-by: Umesh Deshpande <[email protected]>
Signed-off-by: Umesh Deshpande <[email protected]>
Signed-off-by: Umesh Deshpande <[email protected]>
Signed-off-by: Umesh Deshpande <[email protected]>
Motivation
PEFT, https://github.com/foundation-model-stack/fms-hf-tuning includes support for tuning LoRA bias. This PR enables bias for lora, so the adapters with bias will work with vLLM.
Changes Included